Skip to content

fix: field value updates may fail multiple times - #754

Merged
zombieJ merged 2 commits into
react-component:masterfrom
lich-yoo:fix/field-change
Jul 27, 2026
Merged

fix: field value updates may fail multiple times#754
zombieJ merged 2 commits into
react-component:masterfrom
lich-yoo:fix/field-change

Conversation

@lich-yoo

@lich-yoo lich-yoo commented Jun 12, 2025 •
edited by coderabbitai Bot
Loading

Copy link
Copy Markdown
Contributor

fix #753

// Field input
props.onChange?.('');
props.onChange?.('A');

// Test
expect(form.getFieldValue('input')).toBe('A');

// Result
expect(received).toBe(expected)
Expected: "A"
Received: ""

Summary by CodeRabbit

  • 缺陷修复
    • 优化表单字段在连续快速变更时的状态计算流程，确保派发与归一化依据最新输入值，避免中间旧值导致最终结果不一致。
  • 测试
    • 新增用例覆盖连续两次 onChange（如先空字符串再输入 "A"）的场景，验证表单最终字段值正确反映最新变更。
    • 移除了测试文件中未使用的导入，精简测试代码。

@vercel

vercel Bot commented Jun 12, 2025 •
edited
Loading

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
field-form ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 12, 2025 3:07am

@coderabbitai

coderabbitai Bot commented Jun 12, 2025 •
edited
Loading

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: 51402a97-bf31-4637-85f7-18abb6a56c37

📥 Commits

Reviewing files that changed from the base of the PR and between f7a487a and fedb527.

📒 Files selected for processing (1)
  • tests/field.test.tsx

Walkthrough

此次更改在 src/Field.tsx 中读取触发元事件后的最新字段值，并将其用于归一化和更新判断；测试覆盖同一事件中的连续字段变更。

Changes

字段值更新

Layer / File(s) Summary
基于最新值处理字段更新
src/Field.tsx, tests/field.test.tsx		 字段处理器使用最新值进行归一化和更新比较；新增测试验证连续触发空字符串与 "A" 后字段最终值为 "A"，并移除未使用的导入。

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: zombiej

Poem

兔子轻敲按钮忙，
字段变更不再忘。
curValue 护航程，
连续更新终成章。
🐇

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed 标题简洁且准确概括了本次修复的核心：字段值在连续更新时可能未能正确刷新。
Linked Issues check ✅ Passed 代码与 #753 的目标一致，修复了连续 onChange 时未使用最新字段值导致更新失效的问题。
Out of Scope Changes check ✅ Passed 改动仅涉及字段更新逻辑及对应测试，没有明显无关的范围外变更。
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

coderabbitai[bot]
coderabbitai Bot reviewed Jun 12, 2025
View reviewed changes

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
src/Field.tsx (1)

608-622: 引入 curValue 合理，但可再压缩一次 getValue 调用

当前实现先在 getControlled 开头计算了 const value = this.getValue();，随后在触发器内再次执行 this.getValue() 得到 curValue。
虽然两次调用都很轻量，但在高频输入场景仍是一次不必要的遍历（getFieldsValue(true) 会克隆整棵 store）。可以直接删除最前面的 value 变量，改为在外层利用 curValue，从而避免一次冗余读取，逻辑上也更统一。

- const value = this.getValue();
+ // 延后到真正渲染子节点时再取值，避免后续再次读取

其余修改（归一化及比较均改用最新值）完全解决了 #753 的时序问题，👍。

tests/field.test.tsx (1)

76-108: 测试覆盖到位，但等待方式仍有潜在隐患

await act(async () => timeout()) 依赖固定 10 ms 定时器，若后续实现改为批量更新或启用 flushSync，该时长可能不足/过多导致偶发失败。
建议改用 Testing Library 提供的 waitFor 或 findBy 系列 API 监听实际 DOM 变动，而不是硬编码时间。

- await act(async () => {
-   await timeout();
- });
+ await waitFor(() =>
+   expect(form.current?.getFieldValue('input')).toBe('A'),
+ );

这样既能保证可靠性，也能让测试速度随实现自动收敛。
代码其余部分无问题，成功重现并验证修复效果。

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6611c31 and cb183e6.

📒 Files selected for processing (2)
  • src/Field.tsx (2 hunks)
  • tests/field.test.tsx (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
tests/field.test.tsx (3)
src/interface.ts (1)
  • FormInstance (254-276)
tests/common/InfoField.tsx (1)
  • Input (9-11)
tests/common/timeout.ts (1)
  • timeout (3-7)

@lich-yoo

lich-yoo commented Jun 16, 2025

Copy link
Copy Markdown
Contributor Author

@zombieJ

@codecov

codecov Bot commented Jul 11, 2025 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.54%. Comparing base (eb30cdb) to head (fedb527).
⚠️ Report is 1 commits behind head on master.

Additional details and impacted files 
@@           Coverage Diff           @@
##           master     #754   +/-   ##
=======================================
  Coverage   99.54%   99.54%           
=======================================
  Files          20       20           
  Lines        1328     1329    +1     
  Branches      329      329           
=======================================
+ Hits         1322     1323    +1     
  Misses          6        6

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@liuchuzhang liuchuzhang mentioned this pull request Jul 25, 2026
Closed
yoyo837
yoyo837 reviewed Jul 27, 2026
View reviewed changes
Comment thread tests/field.test.tsx Outdated
const MockBtnInput = props => (
<>
<Input {...props} />
<button

@yoyo837 yoyo837 Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<button
<button type="button"

@lich-yoo lich-yoo Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

其他 button 也没加 type，看起来不是必要的

yoyo837
yoyo837 reviewed Jul 27, 2026
View reviewed changes
Comment thread tests/field.test.tsx Outdated
Comment on lines +103 to +105
await act(async () => {
await timeout();
});

@yoyo837 yoyo837 Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
await act(async () => {
await timeout();
});

这个应该不必要吧？

@lich-yoo lich-yoo Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

从其他测试用例抄的。从感官上觉得可以留着，避免误以为更新时序问题

@yoyo837

yoyo837 commented Jul 27, 2026

Copy link
Copy Markdown
Member

再rebase下重新跑下CI

@vercel

vercel Bot commented Jul 27, 2026

Copy link
Copy Markdown

Someone is attempting to deploy a commit to the React Component Team on Vercel.

A member of the Team first needs to authorize it.

@yoyo837
yoyo837 requested a review from zombieJ July 27, 2026 09:08
@zombieJ
zombieJ merged commit f9437eb into react-component:master Jul 27, 2026
3 of 5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yoyo837 yoyo837 yoyo837 approved these changes
@coderabbitai coderabbitai[bot] coderabbitai[bot] left review comments
@zombieJ zombieJ Awaiting requested review from zombieJ
+1 more reviewer
@liuchuzhang liuchuzhang liuchuzhang approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Field onChange 概率性更新不成功

4 participants

@lich-yoo @yoyo837 @liuchuzhang @zombieJ